ci: key E2E workflow_dispatch concurrency on head_sha - #1367
Conversation
The Forge gate counted the old group string (0==2 after the workflow change). CI E2E's guard only required github.ref, which is how two dispatches shared refs/heads/main and cancelled each other.
zoroyihan7
left a comment
zoroyihan7
left a comment
There was a problem hiding this comment.
LGTM — approving. This is the fix for a bug that was actively burning GPU hours, and the diagnosis and the key are both right.
For context on why this matters: I hit this twice from both sides. A dispatch I made for #1322 ran ~58 minutes and was cancelled 18 seconds after a dispatch for #1325 was created; later my dispatch for #1322 killed one of yours that had been queued for 2h51m. Every workflow_dispatch collapsed to ci-e2e-refs/heads/main, and since dispatch is the only way to get a real e2e on a fork PR (no secrets on pull_request), two fork PRs could never both be validated — and the loser vanished silently.
What I verified
inputs.head_shais safe on non-dispatch events. This was my main worry, since the workflow-levelconcurrencyexpression is evaluated before any job'sif, so a bad reference would break run creation for every event. Empirically fine: thepull_request-triggeredCI E2EandForge E2Eruns on this very branch both completed successfully with the new expression.resolvereally does exporthead_shain both workflows, so the forge-e2e side isn't silently falling through tohead_ref.- The keys behave correctly at the boundaries. Two dispatches for different SHAs land in different groups (the fix); two for the same SHA still supersede each other (desirable). The
noop-commentguard is untouched and still first in the chain. - Merged
mainlocally — this branch is 121 commits behind and #1376 touched bothforge-e2e.ymlandtest_forge_e2e_gate.py. Merges clean, fix intact, 15 passed, andscripts/tests/plus the guard file are 195 passed. - Mutation-tested it. Reverting the ci-e2e group, reverting both forge-e2e groups, and moving
head_shaaftergithub.refeach turn a test red. So the tests genuinely pin the change rather than just describing it.
One hole, non-blocking
Deleting head_sha: ${{ steps.decide.outputs.head_sha }} from resolve.outputs in forge-e2e.yml leaves all 15 tests green. The group expression then silently falls through to head_ref, and the bug is back with CI still passing — which is the same silent-fallthrough shape this PR exists to fix. The tests string-match the consumer (the group expression) but never assert the producer it depends on.
One line in test_forge_e2e_gate.py closes it:
assert "head_sha: ${{ steps.decide.outputs.head_sha }}" in workflowWorth adding, but I don't want to hold up a fix that's costing real GPU time. Take it here or as a follow-up.
3 participants
Summary
workflow_dispatchhas nopull_request.number/issue.number, so CI E2E concurrency fell through togithub.ref(refs/heads/mainfor every dispatch).cancel-in-progress: truethen killed the previous GPU smoke — #1325's dispatch cancelled #1322's run 33463143636 after ~58 min on the GPU.This keys the group on
inputs.head_sha(theninputs.head_ref) beforegithub.ref, so two fork-PR dispatches can coexist. Same idea on Forge E2E job concurrency (head_shabeforehead_ref).forge-kernel-bench.ymlalready uses a singleforge-kernel-benchgroup withcancel-in-progress: false, so dispatches serialize instead of killing each other. Left it alone.Test plan
head_shas frommainand confirm the first run is not cancelled/retest/retestlabel path still cancels the same-PR in-flight run